Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: finality provider phase 1 to phase 2 documentation for operators #130

Merged
merged 58 commits into from
Nov 29, 2024

Conversation

samricotta
Copy link
Contributor

Summary

Rework of documentation for users to transition from phase 1 to 2 and includes also how to set up a FP from scratch.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines 231 to 246
## Loading Existing Keys (Only for Phase 1 Finality Providers)

Note: This section is only for users who participated in Phase 1 and already
have an EOTS key. If you are a new user, you can skip this section.

If you participated in Phase 1, follow these steps to load your existing EOTS
key and configure it for Phase 2.

### Step 1: Verify Your EOTS Key Backup
Before proceeding, ensure you have a backup of your Phase 1 EOTS key.
You will need this backup file to import the key into the Phase 2 environment.

### Step 2: Import Your EOTS Key into the Keyring
To load your existing EOTS key, use the following command to import it into the
keyring:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is TBC, need to confirm and is still being developed

docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
Bitcoin network (for security) and the Babylon network (for rewards and
governance).

## Slashing Conditions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also have an explainer on:

  • Jailing and unjailing
  • Public randomness submission
  • Reading the logs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically what would you want to add on Public Randomness Commit? as @gitferry already wrote a doc on committing here.

Are we wanting a reference to the process after committing? For example when signing a block then retrieving the specific randomness?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nice one @gitferry ! We can just reference the doc then. Maybe we could have a nice graphic that explains the life cycle of the finality provider, including creation, registration, randomness commits, voting, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scope of this doc is fore migration and registration, so for other operations like how the fp works, how to unjail we should have reference to them. Docs we already have:

  • fp status transition: ./docs/fp-core.md
  • public randomness commit: ./docs/commit-pub-rand.md
  • send finality votes: ./docs/send-finality-vote.md

Docs we are missing (let's leave todos for them):

  • Unjail instruction
  • Slashing protection
  • Withdraw (or Claim) rewards

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't already have but this can be something I can work on and I agree they can be linked here.

@vitsalis
Copy link
Member

Also, is this going to replace the existing documentation? If so, maybe the PR should delete them.

Copy link
Member

@gitferry gitferry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work composing up everything. Some initial comments:

docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
Comment on lines 423 to 424
- Generates a BTC public key that uniquely identifies your finality provider
- Creates a Babylon account to receive staking rewards
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid creating keys in this cmd. EOTS keys should be created via eotsd keys add and Babylon account should be created via fpd keys add. I was thinking that we should introduce eots-pk flag as required to identify the EOTS key to sign pop.

Therefore, the purpose of this cmd is to prepare all the information (description and generating pop) locally for future registration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just triple checking (again) this is the right command though: create-finality-provider and this won't add the babylon account automatically?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current implementation create-finality-provider will create eots key and babylon account if --keyname does not exist, but we should change it to not duplicate functionalities with keys add

docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
docs/finality-provider-phase2.md Outdated Show resolved Hide resolved
Comment on lines 581 to 582
When withdrawing rewards, you need to use the Babylon chain's CLI since rewards
are managed by the main chain.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would have a cmd within fpd after addressing #13.

@samricotta
Copy link
Contributor Author

Also, is this going to replace the existing documentation? If so, maybe the PR should delete them.

@vitsalis the options are we either

  • have this page replace the main page and add a small summary of what a finality provider is at the top of the page
  • OR just have the main page and remove all of the installation steps and have it as just explanation of what a finality provider is.

Copy link
Member

@gitferry gitferry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work! Thanks for all the back-and-forth. Mostly minor comments. Some of them can be addressed in a future pr.

We should update CHANGELOG.md to make CI happy

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Comment on lines 529 to 530
If one is not provided the finality provider will request
the creation of a new one from the connected EOTS manager instance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should cut

docs/finality-provider-operation.md Show resolved Hide resolved
docs/finality-provider-operation.md Show resolved Hide resolved
docs/finality-provider-operation.md Outdated Show resolved Hide resolved
```

You can verify the transaction was successful by looking up this transaction
hash on the Babylon chain.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we provide a link to a third party explorer or CLI cmd to query the tx content?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vitsalis what do you think as we already took the CLI cmd out for this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think we should link to something here, not the purpose of this guide. This is just a small tip.

I think including the below TODO should be enough in terms of verification

Copy link
Member

@filippos47 filippos47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc is very very good already!
Let me know if you'll add the Prometheus metrics in this PR to review again.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Various comments here and there. Happy to not resolve all of them before merging this PR as long as they are tracked as issues.

Let's verify whether we can support encrypted keyrings for the EOTS manager before merging this PR. If we can only support test I have outlined various spots that we need to update our wording to reflect that on Section 3.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
--home ./fpHome
```

<!-- TODO: JSON file -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this TODO be tracked as an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is here #173

Comment on lines 523 to 524
> finality providers. This could lead to slashing. Only use this if you have
> multiple finality providers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> finality providers. This could lead to slashing. Only use this if you have
> multiple finality providers.
> finality providers. This could lead to slashing.

Only use what?


Required parameters:
- `--chain-id`: The Babylon chain ID (e.g., for the testnet, `bbn-test-5`)
- `--eots-pk`: The EOTS public key maintained by the connected EOTS manager
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gitferry does the EOTS manager provide protections against people using the same --eots-pk? Worry this might be misused leading to slashing or wasting randomness.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • fpd will not allow create a fp if same eots pk already exists in db
  • create-finality-provider will let EOTS manager sign pop (Schnorr sign not EOTS sign). Even if it is the same eots pk, signing over different babylon address will not cause slashing. Why would it relate to randomness?

docs/finality-provider-operation.md Outdated Show resolved Hide resolved
hash on the Babylon chain.


<!-- vitsalis: TODO: How about listing the finality providers using the CLI to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this TODO?

Copy link
Contributor Author

@samricotta samricotta Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has an issue which can be found here #172

@samricotta samricotta merged commit 8fd17f7 into main Nov 29, 2024
10 checks passed
@samricotta samricotta deleted the sam/docs-fp branch November 29, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants